-
-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Prepare for Android alpha + refactor the SDK to be used in conjunction with @sentry/angular #35
Conversation
e5fe716
to
a34f025
Compare
@@ -8,7 +8,9 @@ | |||
android:label="@string/app_name" | |||
android:roundIcon="@mipmap/ic_launcher_round" | |||
android:supportsRtl="true" | |||
android:theme="@style/AppTheme"> | |||
android:theme="@style/AppTheme" | |||
android:usesCleartextTraffic="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a sample so it does not matter but are we calling an HTTP endpoint instead of HTTPS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had to add this to get the capacitor dev server to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit but LGTM
@@ -62,14 +62,15 @@ public void startWithOptions(final PluginCall capOptions) { | |||
SentryAndroid.init( | |||
this.getContext(), | |||
options -> { | |||
if (capOptions.getData().has("debug") && capOptions.getBoolean("debug")) { | |||
options.setDebug(true); | |||
logger.setLevel(Level.INFO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually have DEBUG
by default. User can lower it if it's too noisy, but out of the box we give as much information as we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.setLevel(Level.INFO); is only within the SentryCapacitor.java
, its not the Android SDK logger, so its fine
@@ -62,14 +62,15 @@ public void startWithOptions(final PluginCall capOptions) { | |||
SentryAndroid.init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: bump the Android SDK to the latest GA, 5.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, after this PR I'll make one to bump Android and another to bump JS
Updates the readme with the new installation for angular and the init method from #35
Refactors the SDK to be used in conjunction with
@sentry/angular
, users will need to install both@sentry/capacitor
and@sentry/angular
if using angular, or just@sentry/capacitor
on its own. Currently only Android is still supported.As this PR contains tons of removals and files, for ease of review, the most notable changes are in
wrapper.ts
,sdk.ts
, andSentryCapacitor.java
.Notable Changes
Sentry.nativeCrash()
.Usage:
For example, when using with angular:
Install via
Set up via
In theory installing alongside
@sentry/react
and@sentry/vue
should work, but I have yet to test this so we don't officially support them yet. #40, #41Source Maps
When using with ionic angular and building via
ionic build
, you can upload thewww
directory with sentry-cli to upload the sourcemaps to de-minify the stack trace.In the near future we should introduce auto source map uploading at bulildtime: #39